Merging `[time.zone]`, `[time.clock]`, and `[time.parse]` pieces of P0355R7 #1789

mnatsuhara · 2021-03-30T00:28:34Z

⌚ ⌛ 🗺️

Partially addresses #12

This merges feature/chrono into main as [time.zone], [time.clock], and [time.parse] have been implemented, leaving no partially-finished pieces except for formatting (see Extensions to <chrono>).

Thanks to @d-winsor, @MattStephanson, and @StephanTLavavej for all of their hard work!

feature/chrono: Merge toolset update

* [time.clocks] C++20 clocks * concepts workarounds for /BE * test failures and review feedback * buildfix * fix _HAS_CXX20/namespace nesting * _File_time_clock to filesystem namespace * [time.clock.file] tests now pass, consolidate testing code * revert whitespace change; more review feedback * utc_clock::to_sys with floating point durations * Unicode Windows API functions * YOU get constexpr, YOU get constexpr, EVERYBODY gets constexpr * Comment cleanup * SHOUTY comments: in for a penny, in for a pound * formatting * static_assert wording * improved exception handling * switch negated test * less macro-y alias * explicit return types to match Standard * renames and noexcept * misc review feedback * clang-format * addressing review comments on test coverage Co-authored-by: Miya Natsuhara <[email protected]>

* Loading time_zone names from icu.dll * formatting * Addressed feedback and fixed tests. * Removed manual link map and altered tests. * 121 chars in test script hacks :( * Try again with tests * Test should run now * I think my previous changes got flagged? * Trying to get output from tests. * Win10 20H2 VMSS. * Reverted CI test hacks * Feedback and error handling. * Version, error handling and feedback * More feedback * Remove qualifiers from test Co-authored-by: mnatsuhara <[email protected]> Co-authored-by: Stephan T. Lavavej <[email protected]> Co-authored-by: mnatsuhara <[email protected]>

* Initial draft * Addressed feedback * Final feedback

Co-authored-by: Stephan T. Lavavej <[email protected]>

StephanTLavavej

Looks good, just noticed some memory leaks and minor nitpicks. I'll push fixes.

stl/src/tzdb.cpp

stl/inc/chrono

tests/std/tests/P0355R7_calendars_and_time_zones_clocks/test.cpp

* Remove extra space in comment. * Consistently _CHRONO qualify "near-shadowing". * Remove now() strengthening; utc_clock::now() isn't strengthened. * Cite WP number.

StephanTLavavej · 2021-03-31T07:22:43Z

I pushed the following changes:

stl/msbuild: Link advapi32.lib.
- This is for the kernel32 and netfx flavors only (netfx is the special copy for the CLR). For the app and onecore flavors, onecore.lib provides the registry APIs.
static_cast from int to bool to avoid warning C4800.
Add run_tz_test() to report exceptions and handle old OSes.
- This adds detailed reporting of system_error::code() if something fails in the future.
- For MSVC_INTERNAL_TESTING only, where the test machines are running Server 2019 (and older!), we detect ERROR_MOD_NOT_FOUND and skip the test. Any other errors are reported (so if/when the internal machines are updated, this will begin providing real coverage). For GitHub (CI and local development), loading icu.dll is not allowed to fail.
Remove _Invalid_time_string to fix the internal objsize test.
- @MattStephanson confirmed that this was no longer used (it was a relic of development). P0355R7_calendars_and_time_zones_io/test.cpp just needs a bogus value, the specific string is unimportant.
Work around VSO-1303556, an is_constructible_v ICE. (Which appears to be an assertion in the internal "chk" builds of the compiler, that doesn't lead to crashes in the public "ret" builds, otherwise we would have seen this earlier.)
- Fully reduced and reported (this is a problem in the type traits optimizer which is why ::value appears to be a reliable workaround). The "circular" constraint of having zoned_time constructors query the constructibility of zoned_time is unusual, but doesn't appear to be wrong.
Fix memory leaks.
- We no longer null out _Info's data members, as we delete _Info itself.
Code review feedback.
- Remove extra space in comment.
- Consistently _CHRONO qualify "near-shadowing".
- Remove now() strengthening; utc_clock::now() isn't strengthened.
- Cite WP number.

miscco

My hat is for for that incredible work. I will happily remember to not do any chrono related stuff.

I found some nits. Generally I really prefer to delete special members rather than defaulting the other ones.

stl/inc/chrono

…into bye_feature_chrono

stl/inc/chrono

Co-authored-by: MattStephanson <[email protected]>

stl/inc/chrono

CaseyCarter

None of these comments relates to correctness. I'm fine with making changes now or filing a followup issue "Address Casey's comments on GH-1789," so I'm approving.

CaseyCarter · 2021-04-01T18:35:15Z

stl/inc/__msvc_tzdb.hpp

+    void deallocate(_Ty* const _Ptr, size_t) noexcept {
+        __std_free_crt(_Ptr);
+    }
+};


I'm disturbed by having a _Meow_allocator type in product code that doesn't satisfy the allocator requirements. It might be overkill to actually implement template <class _Ty1, class _Ty2> bool operator==(const _Crt_allocator<_Ty1>&, const _Crt_allocator<_Ty2>&), however - could we at least have a comment to the effect that this isn't really an allocator because == is missing?

I'd prefer to have a properly implemented == (as product code should hold itself to a high standard), even though we aren't actually using it yet.

CaseyCarter · 2021-04-01T20:54:08Z

stl/inc/chrono

+    // CLASS time_zone
+    class time_zone {
+    public:
+        explicit time_zone(string_view _Name_) : _Name(_Name_) {}


Should we make this constructor which never emits exceptions noexcept? (Note that this wouldn't be a strengthening since this constructor is unspecified.)

Speaking of unspecified, should this constructor (and all similar unspecified constructors for other types) have a first parameter with an internal type so users cannot accidentally (or intentionally) construct one from a string_view?

+1 for both. Upon looking at this again, the potential for users to write time_zone tz{"PDT"}; is dangerously high.

Upon looking at this again, the potential for users to write time_zone tz{"PDT"}; is dangerously high.

Yes, it would be nice to make it easier for users to avoid accidentally (or intentionally) using a non-portable initialization.

I noticed that the data member we're constructing is string _Name;, so noexcept would be unwise here.

stl/inc/chrono

CaseyCarter · 2021-04-01T21:44:35Z

stl/inc/chrono

+    // CLASS time_zone_link
+    class time_zone_link {
+    public:
+        explicit time_zone_link(string_view _Name_, string_view _Target_) : _Name(_Name_), _Target(_Target_) {}


Ditto "Should we noexcept?"

Ditto nope, we're constructing string _Name; and string _Target;.

We can tag this constructor, though.

CaseyCarter · 2021-04-01T21:53:57Z

stl/inc/chrono

+            _Xruntime_error("Internal error loading IANA database information");
+        }
+
+        return {_Info->_Tz_name};


While this usage is correct, brace-init of a type with an initializer_list constructor (or a type that potentially has such a constructor) when we don't intend to invoke that initializer_list constructor is a code smell. Could we just return _Info->_Tz_name; here so every future reader doesn't have to verify that this is ok?

CaseyCarter · 2021-04-05T21:18:37Z

stl/inc/chrono

+
+        template <class _DurationType>
+        _NODISCARD bool _Make_time_point(_DurationType& _Dur, _LeapSecondRep _Leap) {
+


Suggested change

CaseyCarter · 2021-04-05T21:34:24Z

stl/inc/chrono

+            if (_Leap != _LeapSecondRep::_None) {
+                if (_Hour_24 == 23 && _Minute == 59 && _Second >= 59) {
+                    // It's possible that the parsed time doesn't exist because (a) _Seconds == 60 and there *isn't* a
+                    // leap second insertion or (b) _Seconds == 59 and there *is* a leap second subtraction.


Line 4133 refers to negative leap second "deletion"; we should consistently use the same term throughout this file. I have no strong feelings, but note that "deletion" is more obviously an antonym of "insertion".

CaseyCarter · 2021-04-05T21:45:25Z

stl/inc/chrono

+        _NODISCARD _InIt _Parse_time_field(_InIt _First, ios_base& _Iosbase, ios_base::iostate& _State,
+            const char _Flag, const char _Modifier, const unsigned int _Width,
+            const unsigned int _Subsecond_precision) {
+            using _CharT = typename _InIt::value_type;


This is not correct if _InIt is a generic input iterator. It should use iterator_traits to retrieve the corresponding value type, or better yet the shortcut alias _Iter_value_t<_InIt>. If it's not a generic input iterator, we should change the name to avoid our convention that _InIt means "input iterator." It looks like these only need to work with istream_iterators; If that's the case, just plain _Iter will work. (Several occurrences.)

CaseyCarter · 2021-04-05T21:47:37Z

stl/inc/chrono

+
+            const auto& _Ctype_fac = _STD use_facet<ctype<_CharT>>(_Iosbase.getloc());
+            const auto& _Time_fac  = _STD use_facet<time_get<_CharT>>(_Iosbase.getloc());
+            constexpr _InIt _Last{};


Cpp17InputIterator does not require default construction - let alone constexpr default construction - so this is plain wrong if _InIt is meant to be a generic input iterator.

It's always an istreambuf_iterator; I'll make the code accordingly specific so this will be obviously correct.

stl/inc/chrono

StephanTLavavej · 2021-04-06T02:43:30Z

Thanks @mnatsuhara, @d-winsor, and @MattStephanson for these major pieces of <chrono>! ⏳ 🎉 🚀

StephanTLavavej and others added 9 commits March 4, 2021 15:25

Merge pull request microsoft#1716 from microsoft/main

5f4c4ba

feature/chrono: Merge toolset update

Logic for time_zone sys to local conversion (microsoft#1732)

2bc79d2

* Initial draft * Addressed feedback * Final feedback

Support for zoned_time (microsoft#1752)

cb2b985

Co-authored-by: Stephan T. Lavavej <[email protected]>

Misc items for tzdb (microsoft#1765)

74abbc2

Co-authored-by: Stephan T. Lavavej <[email protected]>

<chrono> [time.parse] (microsoft#1768)

027fc9f

Co-authored-by: Stephan T. Lavavej <[email protected]>

chrono cleanups (microsoft#1779)

3017b91

<chrono>: Avoid unnecessary use of concepts (microsoft#1787)

dde4623

mnatsuhara requested a review from a team as a code owner March 30, 2021 00:28

mnatsuhara added chrono C++20 chrono cxx20 C++20 feature labels Mar 30, 2021

StephanTLavavej changed the title ~~Merging [time.zone] and [time.clock] pieces of P0355R7~~ Merging [time.zone], [time.clock], and [time.parse] pieces of P0355R7 Mar 30, 2021

StephanTLavavej self-assigned this Mar 30, 2021

StephanTLavavej reviewed Mar 31, 2021

View reviewed changes

StephanTLavavej added 7 commits March 30, 2021 23:48

stl/msbuild: Link advapi32.lib.

6bcb4eb

static_cast from int to bool to avoid warning C4800.

0c34af6

Add run_tz_test() to report exceptions and handle old OSes.

37c43ed

Remove _Invalid_time_string to fix the objsize test.

b0f132d

Work around VSO-1303556, an is_constructible_v ICE.

fd6ecdb

Fix memory leaks.

aa54dc4

Code review feedback.

620d71a

* Remove extra space in comment. * Consistently _CHRONO qualify "near-shadowing". * Remove now() strengthening; utc_clock::now() isn't strengthened. * Cite WP number.

StephanTLavavej approved these changes Mar 31, 2021

View reviewed changes

StephanTLavavej removed their assignment Mar 31, 2021

CaseyCarter self-assigned this Mar 31, 2021

miscco reviewed Apr 1, 2021

View reviewed changes

mnatsuhara added 5 commits April 1, 2021 11:33

extract time_since_epoch()

213fced

early returns

e885fd0

small restructuring

cbadcdf

pull _For_time into an enum template parameter

a6b6a2c

Merge branch 'bye_feature_chrono' of https://github.com/mnatsuhara/STL …

10b9a70

…into bye_feature_chrono

This comment has been minimized.

Sign in to view

StephanTLavavej added 2 commits April 1, 2021 17:58

Refactor another early-ish return.

e83116e

Reorder _Parse_tp_or_duration, deducing _DurationType.

c5a46da

StephanTLavavej reviewed Apr 2, 2021

View reviewed changes

stl/inc/chrono Outdated Show resolved Hide resolved

stl/inc/chrono Outdated Show resolved Hide resolved

StephanTLavavej approved these changes Apr 2, 2021

View reviewed changes

Test a later leap second.

20fea0a

Co-authored-by: MattStephanson <[email protected]>

StephanTLavavej approved these changes Apr 2, 2021

View reviewed changes

miscco reviewed Apr 2, 2021

View reviewed changes

stl/inc/chrono Show resolved Hide resolved

This comment has been minimized.

Sign in to view

CaseyCarter approved these changes Apr 6, 2021

View reviewed changes

StephanTLavavej mentioned this pull request Apr 6, 2021

<chrono>: Perform cleanups identified during code review #1805

Closed

StephanTLavavej unassigned CaseyCarter Apr 6, 2021

StephanTLavavej merged commit fb2f89f into microsoft:main Apr 6, 2021

StephanTLavavej mentioned this pull request Apr 6, 2021

feature/format: Merge chrono #1807

Merged

mnatsuhara deleted the bye_feature_chrono branch April 9, 2021 18:42

StephanTLavavej mentioned this pull request Apr 21, 2021

<chrono> Formatting: C++20's Final Boss #1870

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merging `[time.zone]`, `[time.clock]`, and `[time.parse]` pieces of P0355R7 #1789

Merging `[time.zone]`, `[time.clock]`, and `[time.parse]` pieces of P0355R7 #1789

mnatsuhara commented Mar 30, 2021 •

edited

Loading

StephanTLavavej left a comment

StephanTLavavej commented Mar 31, 2021

miscco left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

CaseyCarter left a comment

CaseyCarter Apr 1, 2021

StephanTLavavej Apr 6, 2021

CaseyCarter Apr 1, 2021

CaseyCarter Apr 1, 2021

StephanTLavavej Apr 6, 2021

CaseyCarter Apr 6, 2021

StephanTLavavej Oct 21, 2023

CaseyCarter Apr 1, 2021

StephanTLavavej Oct 21, 2023

CaseyCarter Apr 1, 2021

CaseyCarter Apr 5, 2021

CaseyCarter Apr 5, 2021

CaseyCarter Apr 5, 2021 •

edited

Loading

CaseyCarter Apr 5, 2021

StephanTLavavej Oct 21, 2023

StephanTLavavej commented Apr 6, 2021


		template <class _DurationType>
		_NODISCARD bool _Make_time_point(_DurationType& _Dur, _LeapSecondRep _Leap) {

Merging [time.zone], [time.clock], and [time.parse] pieces of P0355R7 #1789

Merging [time.zone], [time.clock], and [time.parse] pieces of P0355R7 #1789

Conversation

mnatsuhara commented Mar 30, 2021 • edited Loading

StephanTLavavej left a comment

Choose a reason for hiding this comment

StephanTLavavej commented Mar 31, 2021

miscco left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

CaseyCarter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaseyCarter Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephanTLavavej commented Apr 6, 2021

Merging `[time.zone]`, `[time.clock]`, and `[time.parse]` pieces of P0355R7 #1789

Merging `[time.zone]`, `[time.clock]`, and `[time.parse]` pieces of P0355R7 #1789

mnatsuhara commented Mar 30, 2021 •

edited

Loading

CaseyCarter Apr 5, 2021 •

edited

Loading